Add grpc unary server interceptor #312

shouichi · 2020-12-18T02:10:23Z

This is a draft PR to support grpc (see #240). The main goal is to gather feedback and agree on the interface. Thus tests are not written yet. Any feedback/comments are welcomed. Thanks!

This project is based heavily on the [grpc-go-middleware libraries][1] and a [pull-request][2] from the [sentry-go library][3]. [1]: https://github.com/grpc-ecosystem/go-grpc-middleware [2]: getsentry/sentry-go#312 [3]: getsentry/sentry-go#240

rhcarvalho

@shouichi thanks for the PR. Sorry, I didn't have the bandwidth to review it over the holiday season.

The current draft looks promising, would love to see some test cases and examples how it will be used.

shouichi · 2021-01-19T06:53:06Z

@rhcarvalho Added tests. Please take a look.

rhcarvalho

Looks very good!

A couple of questions / suggestions. I think we can merge this soon, thank you!

grpc/server.go

grpc/server_test.go

grpc/server.go

Also adresse other minor/style issues.

shouichi

@rhcarvalho Changed to use the functional options pattern. Please take a look.

shouichi · 2021-01-20T07:20:46Z

grpc/options.go

+}
+
+// Option configures reporting behavior.
+type Option func(*options)


I wonder if we should separate options for server/client and unary/stream. Probably they won't diverge too much?

Good point, ~~perhaps what would give us more flexibility is~~

Suggested change

type Option func(*options)

type Option interface{

apply(*options)

}

~~Later down the road it could become~~

type Option interface{ applyServer(*serverOptions) applyClient(*clientOptions) // ... }

It would allow us to share options between servers and clients, unary and stream.
Though I'm not sure that's actually a good idea. One downside is discoverability and documentation. If server/client/etc options have the same type, all of them will be grouped together in godoc. If an option doesn't make sense to a certain type (client/server/unary/stream), that would also require documentation to explain instead of being impossible to compile because of incompatible types.

I think the options we have right now are "interceptor options", and should probably be the same for other types of interceptors? Are there other existing concepts we'd expand into?

Suggested change

type Option func(*options)

type InterceptorOption interface{

apply(*options)

}

If there is only reason to be one type of option, for interceptors, then we can keep the name simple, Option.

Sorry, busy day, didn't have much time to think this through but wanted to share and continue the conversation.

I'm not really sure but both of the following changes shouldn't break user code because they don't directly use Option. So can we start simple?

Changing type Option func(*options) to type Option interface {}.

Renaming Option to InterceptorOption.

Sorry, busy day, didn't have much time to think this through but wanted to share and continue the conversation.

No worries! We don't need to rush. I (we?) just want to agree on a good design ;)

grpc/options.go

Co-authored-by: Aubrey Dean <[email protected]>

shouichi · 2021-02-16T05:48:06Z

@rhcarvalho @spaceweasel Hi, it's been a while but could you take another look at this PR?

becoded · 2021-02-26T16:54:29Z

I was looking for this. Glad to see it's only a review away from getting it merged!

odannyc · 2021-04-22T21:14:41Z

Hi, it's been some time since this has been open. Are there plans to merge this in?

@rhcarvalho @spaceweasel

vesquen · 2021-07-06T11:33:35Z

This is exactly what I'm looking for. The pull request has been open for a while. Any chance this still can get a review? @rhcarvalho @spaceweasel

koteesy · 2021-07-11T11:43:14Z

Any news?

serbanmarti · 2021-07-14T08:48:22Z

Really looking forward to this getting merged!

legosx · 2021-07-20T11:58:58Z

Can't wait to see it's merged. This is exactly what I was looking for! Thanks a lot

prasad-marne · 2021-08-20T05:11:05Z

Any updates. If it needs any help, i can do it.

pablodz · 2021-10-03T03:22:57Z

Still relevant, please update status

flyte · 2021-12-08T14:00:43Z

Could this maybe be turned into a separate package if it's not going to be merged?

HurSungYun · 2022-05-31T02:45:34Z

there's a seperate package here but code is not exactly same.

https://github.com/johnbellone/grpc-middleware-sentry

using this package until this PR merged can be a temporal solution.

github-actions · 2022-12-07T09:43:33Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

shouichi mentioned this pull request Jan 18, 2021

gRPC interceptors #240

Open

rhcarvalho reviewed Jan 18, 2021

View reviewed changes

Add grpc unary server interceptor

a960775

shouichi force-pushed the grpc branch from be2db02 to a960775 Compare January 19, 2021 06:44

shouichi marked this pull request as ready for review January 19, 2021 06:50

rhcarvalho reviewed Jan 19, 2021

View reviewed changes

grpc/server.go Outdated Show resolved Hide resolved

grpc/server.go Show resolved Hide resolved

grpc/server_test.go Outdated Show resolved Hide resolved

grpc/server_test.go Show resolved Hide resolved

grpc/server_test.go Show resolved Hide resolved

rhcarvalho reviewed Jan 19, 2021

View reviewed changes

grpc/server_test.go Outdated Show resolved Hide resolved

grpc/server.go Outdated Show resolved Hide resolved

Use the functional options pattern to configure interceptor

456ebca

Also adresse other minor/style issues.

shouichi force-pushed the grpc branch from 08fc7cd to 456ebca Compare January 20, 2021 07:28

shouichi commented Jan 20, 2021

View reviewed changes

spaceweasel reviewed Jan 20, 2021

View reviewed changes

grpc/options.go Outdated Show resolved Hide resolved

Use map to lookup status code

93183e7

Co-authored-by: Aubrey Dean <[email protected]>

shouichi force-pushed the grpc branch from 7715b32 to 93183e7 Compare January 21, 2021 00:36

shouichi requested a review from rhcarvalho February 1, 2021 06:19

rhcarvalho added the Scope: Integration Package label Oct 11, 2021

hugosantos mentioned this pull request Mar 29, 2022

add sentry support namespacelabs/foundation#62

Closed

cleptric linked an issue Oct 30, 2022 that may be closed by this pull request

gRPC interceptors #240

Open

github-actions bot added the Status: Stale label Dec 7, 2022

cleptric added Type: Feature Status: Backlog and removed Scope: Integration Package labels Dec 9, 2022

cleptric requested review from ribice and removed request for rhcarvalho March 14, 2024 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grpc unary server interceptor #312

Add grpc unary server interceptor #312

shouichi commented Dec 18, 2020

rhcarvalho left a comment

shouichi commented Jan 19, 2021

rhcarvalho left a comment

shouichi left a comment

shouichi Jan 20, 2021

rhcarvalho Jan 20, 2021

shouichi Jan 21, 2021

shouichi commented Feb 16, 2021

becoded commented Feb 26, 2021

odannyc commented Apr 22, 2021

vesquen commented Jul 6, 2021

koteesy commented Jul 11, 2021

serbanmarti commented Jul 14, 2021

legosx commented Jul 20, 2021 •

edited

Loading

prasad-marne commented Aug 20, 2021

pablodz commented Oct 3, 2021

flyte commented Dec 8, 2021

HurSungYun commented May 31, 2022

github-actions bot commented Dec 7, 2022

-type Option func(*options)
+type Option interface{
+	apply(*options)
+}

Add grpc unary server interceptor #312

Are you sure you want to change the base?

Add grpc unary server interceptor #312

Conversation

shouichi commented Dec 18, 2020

rhcarvalho left a comment

Choose a reason for hiding this comment

shouichi commented Jan 19, 2021

rhcarvalho left a comment

Choose a reason for hiding this comment

shouichi left a comment

Choose a reason for hiding this comment

shouichi Jan 20, 2021

Choose a reason for hiding this comment

rhcarvalho Jan 20, 2021

Choose a reason for hiding this comment

shouichi Jan 21, 2021

Choose a reason for hiding this comment

shouichi commented Feb 16, 2021

becoded commented Feb 26, 2021

odannyc commented Apr 22, 2021

vesquen commented Jul 6, 2021

koteesy commented Jul 11, 2021

serbanmarti commented Jul 14, 2021

legosx commented Jul 20, 2021 • edited Loading

prasad-marne commented Aug 20, 2021

pablodz commented Oct 3, 2021

flyte commented Dec 8, 2021

HurSungYun commented May 31, 2022

github-actions bot commented Dec 7, 2022

legosx commented Jul 20, 2021 •

edited

Loading